Session GridField state manager #11288

forsdahl · 2024-06-20T12:49:07Z

Description

Provides a new session-based GridField state manager, for storing all affected GridField states into session instead of in request variables.
Might be causing linting issues from the new required methods not being present in the current GridFieldStateManagerInterface interface, so manual checks have to be done for checking the state manager implementation for them.

Manual testing steps

Can be tested in a standard Silverstripe 5.x installation by adding the following yaml config:

---
Name: customgridfieldconfig
After: gridfieldconfig
---
SilverStripe\Core\Injector\Injector:
  SilverStripe\Forms\GridField\GridFieldStateManagerInterface:
    class: SilverStripe\Forms\GridField\SessionGridFieldStateManager

Issues

Session based GridField state management #11255

Pull request checklist

The target branch is correct
- See picking the right version
All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
The commit messages follow our commit message guidelines
The PR follows our contribution guidelines
Code changes follow our coding conventions
This change is covered with tests (or tests aren't necessary for this change)
Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
CI is green

GridField states in session.

current gridfield state manager configuration.

…methods will be incorporated into GridFieldStateManagerInterface in the future.

GuySartorelli · 2024-06-24T02:17:11Z

I haven't done a full review yet but nothing in the code stands out as obviously bad. I'll aim to do a proper review in the next couple of days. In the meantime, there's some PHP unit test failures which look relevant.

Might be causing linting issues from the new required methods not being present in the current GridFieldStateManagerInterface interface

What linting issues do you mean? The CI PHP linting job is happy with it.

GuySartorelli

Had a quick look and it broadly looks good. I haven't checked every scenario yet. I did find a couple of things:

Existing class with similar funcitonality?

There is already a SilverStripe\Forms\GridField\FormAction\SessionStore class which seems to be storing and fetching some state related to GridField in the session - have you looked at that class at all to see how it relates to this new feature?

Child gridfield state is too sticky

In a scenario where you have a parent DataObject model (I'll call this Parent) in a ModelAdmin, and that has a many_many relation to another model (I'll call it Child):

Filter or sort the Parent gridfield inside the ModelAdmin
Click into one of the Parent records
Filter or sort the Child gridfield
Go back to the Parent gridfield inside ModelAdmin (either click back in the browser or use the breadcrumbs)
Click on the same Parent record again and view the Child gridfield

With the session-based state, the child gridfield is still filtered/sorted when I view it again.

With the existing URL-based state, the child gridfield is reset to its default state. It's debatable, but to me this is the desirable behaviour. It would confuse me if the child gridfield is always filtered whenever I am clicking through the parent record to get to it.

GuySartorelli · 2024-06-24T22:31:50Z

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php

            $actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));
+            if (ClassInfo::hasMethod($manager, 'getStateRequestVar')) {
+                $stateRequestVar = $manager->getStateRequestVar();
+                $stateValue = $this->getRequest()->requestVar($stateRequestVar);
+                if ($stateValue) {
+                    $actions->push(HiddenField::create($stateRequestVar, '', $stateValue));


Do we need both of these hidden fields? Can you please give me a short rundown of what they each do?

We do not really need both no.
The old one is there for backwards compatibility with the default GridStateManager. That one contains the whole grid field state as json data.
The new one (from the getStateRequestVar method) needs a different value, i.e. the given state key value from the request. It is needed with SessionGridFieldStateManager to retain the session state key when for example saving a record.

…onality

forsdahl · 2024-06-27T13:17:36Z

Child gridfield state is too sticky

Retaining that Child state is needed in some cases, for example when going one level further. If you filter the Child gridfield, and then click one record for editing and go back, it needs to retain its state in that scenario.
I agree that it would be preferred if it didn't retain the state when going back one step and opening the same record again, but it is unclear how to separate that scenario from the one where retaining the state is needed. So far retaining the state has had higher priority for us than the scenario where it feels too sticky.

forsdahl · 2024-06-27T13:23:35Z

Existing class with similar funcitonality?

I have looked at SilverStripe\Forms\GridField\FormAction\SessionStore yes, it is used for GridField_FormAction states. It could be used for actually saving and reading values from the current session (which is all it does), but would perhaps result in more code, and coupling between two otherwise unrelated classes.

forsdahl · 2024-06-27T13:25:00Z

What linting issues do you mean? The CI PHP linting job is happy with it.

Seems the CI linter doesn't mind that syntax like the linter in my IDE does, so no problem there after all.

GuySartorelli · 2024-07-07T22:24:01Z

I agree that it would be preferred if it didn't retain the state when going back one step and opening the same record again, but it is unclear how to separate that scenario from the one where retaining the state is needed. So far retaining the state has had higher priority for us than the scenario where it feels too sticky.

Is there a clean way to reset the state for all gridfields other than the current one and its "parents" when clicking on a child row?

If not, it's probably okay for CMS 5 since this is opt-in, but I wouldn't want to go ahead with #11267 with this sticky behaviour - and the whole point of this is to do #11267.

GuySartorelli · 2024-07-07T22:19:46Z

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php

            $actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));
+            if (ClassInfo::hasMethod($manager, 'getStateRequestVar')) {
+                $stateRequestVar = $manager->getStateRequestVar();
+                $stateValue = $this->getRequest()->requestVar($stateRequestVar);
+                if ($stateValue) {
+                    $actions->push(HiddenField::create($stateRequestVar, '', $stateValue));
+                }
+            }


As per #11288 (comment)

Suggested change

$actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));

if (ClassInfo::hasMethod($manager, 'getStateRequestVar')) {

$stateRequestVar = $manager->getStateRequestVar();

$stateValue = $this->getRequest()->requestVar($stateRequestVar);

if ($stateValue) {

$actions->push(HiddenField::create($stateRequestVar, '', $stateValue));

}

}

if (ClassInfo::hasMethod($manager, 'getStateRequestVar')) {

$stateRequestVar = $manager->getStateRequestVar();

$stateValue = $this->getRequest()->requestVar($stateRequestVar);

if ($stateValue) {

$actions->push(HiddenField::create($stateRequestVar, '', $stateValue));

}

} else {

$actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));

}

forsdahl added 4 commits June 20, 2024 15:12

NEW Initial SessionGridFieldStateManager implementation, for managing

4714a83

GridField states in session.

Added Unit Tests for SessionGridFieldStateManager

d448708

Fixed SessionGridFieldStateManager unit test to work regardless of

e9cd597

current gridfield state manager configuration.

Removed custom interface for SessionGridFieldStateManager, the extra …

898ef9b

…methods will be incorporated into GridFieldStateManagerInterface in the future.

GuySartorelli mentioned this pull request Jun 24, 2024

Session based GridField state management #11255

Open

GuySartorelli requested changes Jun 24, 2024

View reviewed changes

GuySartorelli self-assigned this Jun 24, 2024

Fix for UnitTest failure with new SessionGridFieldStateManager functi…

6b69db1

…onality

GuySartorelli requested changes Jul 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Session GridField state manager #11288

Session GridField state manager #11288

forsdahl commented Jun 20, 2024 •

edited by GuySartorelli

Loading

GuySartorelli commented Jun 24, 2024

GuySartorelli left a comment •

edited

Loading

GuySartorelli Jun 24, 2024

forsdahl Jun 27, 2024

forsdahl commented Jun 27, 2024 •

edited

Loading

forsdahl commented Jun 27, 2024

forsdahl commented Jun 27, 2024

GuySartorelli commented Jul 7, 2024 •

edited

Loading

GuySartorelli Jul 7, 2024

Session GridField state manager #11288

Are you sure you want to change the base?

Session GridField state manager #11288

Conversation

forsdahl commented Jun 20, 2024 • edited by GuySartorelli Loading

Description

Manual testing steps

Issues

Pull request checklist

GuySartorelli commented Jun 24, 2024

GuySartorelli left a comment • edited Loading

Choose a reason for hiding this comment

Existing class with similar funcitonality?

Child gridfield state is too sticky

GuySartorelli Jun 24, 2024

Choose a reason for hiding this comment

forsdahl Jun 27, 2024

Choose a reason for hiding this comment

forsdahl commented Jun 27, 2024 • edited Loading

forsdahl commented Jun 27, 2024

forsdahl commented Jun 27, 2024

GuySartorelli commented Jul 7, 2024 • edited Loading

GuySartorelli Jul 7, 2024

Choose a reason for hiding this comment

forsdahl commented Jun 20, 2024 •

edited by GuySartorelli

Loading

GuySartorelli left a comment •

edited

Loading

forsdahl commented Jun 27, 2024 •

edited

Loading

GuySartorelli commented Jul 7, 2024 •

edited

Loading